Repair vaultUpgrade proposal; also register replacement scaledPriceAuthority #9748

Chris-Hibbert · 2024-07-20T00:23:22Z

closes: #9584
refs: #9483

Description

We added upgrading the scaledPriceAuthority to the steps in upgrading vaults, auctions, and priceFeeds, and didn't notice that it broke things. The problem turned out to be that the "priceAuthority" object registered with the priceFeedRegistry was an ephemeral object that was not upgraded. This fixes that by re-registering the new priceAuthority.

Then, to simplify the process of cleaning up the uncollected cycles reported in #9483, we switch to replacing the scaledPriceAuthorities rather than upgrading them.

Security Considerations

N/A

Scaling Considerations

Addresses one of our biggest scaling concerns.

Documentation Considerations

None

Testing Considerations

Thorough testing in a3p.

Upgrade Considerations

See above.

cloudflare-workers-and-pages · 2024-07-31T20:05:15Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`659c016`
Status:	✅ Deploy successful!
Preview URL:	https://d9d92eb9.agoric-sdk.pages.dev
Branch Preview URL:	https://9584-upgradevaults.agoric-sdk.pages.dev

View logs

turadg

Requesting changes but using Comment because I might not be around to re-review

golang/cosmos/app/upgrade.go

packages/inter-protocol/src/proposals/replace-scaledPriceAuthorities.js

turadg · 2024-07-31T20:54:43Z

packages/zoe/src/contractSupport/priceAuthority.js

@@ -259,7 +258,7 @@ export const makeOnewayPriceAuthorityKit = opts => {
  };

  /** @type {PriceAuthority} */
-  const priceAuthority = Far('PriceAuthority', {
+  const priceAuthority = Far('PriceAggregator', {


There are multiple objects going by the name "priceAggregator", or being called priceAggregator, and when I see them in a debugger or trace statement, I'd like to be able to distinguish them.

It does have some appeal. PriceAuthority is an interface, and PriceAggregator is an implementation of it. Being more specific about the implementation seems reasonable... even though the argument to Far is called interface in various contexts.

I wrote priceAggregator when I meant priceAuthority. The latter is the name that is overused, and the former is a name that only applies to this instance.

turadg · 2024-07-31T20:55:06Z

packages/zoe/src/contractSupport/priceAuthorityInitial.js

@@ -110,7 +110,7 @@ export const makeInitialTransform = (
      : quoteP;
  };

-  return Far('PriceAuthority', {
+  return Far('ScaledPriceAuthority', {


why not PriceAuthorityInitial like on the tin?

This object is the one that gets registered, and is used by many contracts. There are multiple things that are called "priceAuthority" in various places in the code (including the priceAuthorityRegistry). When I look at one of these, its extremely helpful to have its name match its purpose.

priceAuthorityInitial is a special case for when an initialPrice is provided in the configuration. You should only see that in testing scenarios. It shouldn't show up in production. The production Far object comes from packages/zoe/src/contractSupport/priceAuthorityTransform.js. So ScaledPriceAuthorityWithInitial would make more sense here.

Okay, then I'll rotate the terms so the one we see in production will be ScaledPriceAuthority.

packages/zoe/src/contracts/scaledPriceAuthority.js

dckc

Testing ... Thorough testing in a3p.

I suppose that's in code that pre-dates this PR...

I'd like to look at the relevant sections of the ci log that show this a3p testing. Help me find it, please?

dckc · 2024-07-31T23:56:33Z

packages/inter-protocol/src/proposals/replace-scaledPriceAuthorities.js

+
+  const [contractKits] = await Promise.all([
+    contractKitsP,
+    instancePrivateArgsP,


why is instancePrivateArgsP here?

dckc · 2024-07-31T23:59:33Z

packages/inter-protocol/src/proposals/replace-scaledPriceAuthorities.js

+  for (const kitEntry of scaledPAKitEntries) {
+    trace({ kitEntry });
+
+    const keyword = kitEntry.label.match(/scaledPriceAuthority-(.*)/)[1];


relying on a label for correctness is awkward.

Did you consider filtering by installation?

I suppose you want the keyword from the label... another way to get that is from the children of published.priceAuthority... oh... but vstorage is write-only on chain. What a pain.

relying on a label for correctness is awkward.

I agree. There seemed few enough vats at present that I could verify this produced the correct list. A slightly longer or more dynamic list and I'd have tried to find a better way.

Did you consider filtering by installation?

I didn't, but it's a good idea. Added.

dckc · 2024-08-01T00:01:31Z

packages/zoe/src/contractSupport/priceAuthority.js

@@ -259,7 +258,7 @@ export const makeOnewayPriceAuthorityKit = opts => {
  };

  /** @type {PriceAuthority} */
-  const priceAuthority = Far('PriceAuthority', {
+  const priceAuthority = Far('PriceAggregator', {


It does have some appeal. PriceAuthority is an interface, and PriceAggregator is an implementation of it. Being more specific about the implementation seems reasonable... even though the argument to Far is called interface in various contexts.

dckc · 2024-08-01T00:07:51Z

packages/zoe/src/contractSupport/priceAuthorityInitial.js

@@ -110,7 +110,7 @@ export const makeInitialTransform = (
      : quoteP;
  };

-  return Far('PriceAuthority', {
+  return Far('ScaledPriceAuthority', {


priceAuthorityInitial is a special case for when an initialPrice is provided in the configuration. You should only see that in testing scenarios. It shouldn't show up in production. The production Far object comes from packages/zoe/src/contractSupport/priceAuthorityTransform.js. So ScaledPriceAuthorityWithInitial would make more sense here.

Chris-Hibbert · 2024-08-01T00:19:33Z

I'd like to look at the relevant sections of the ci log that show this a3p testing. Help me find it, please?

That should be Integration tests / test-docker-build (pull_request), which says Skipped. Is there a label I can apply to get this to run?

Chris-Hibbert · 2024-08-01T16:28:42Z

@dckc The CI log you're looking for is test-docker-build. (Thanks, @turadg!)

dckc · 2024-08-07T17:32:43Z

test-docker-build has a huge log. which parts are relevant?

Chris-Hibbert · 2024-08-08T00:21:35Z

test-docker-build has a huge log. which parts are relevant?

From my POV, the fact that it gets to upgrading after delay is crucial. That's the piece that was missed in the previous round.

We started new auction, priceFeed, and scaledPriceAuthority vats, and then prepared the vaults upgrade and waited
When the proposal noticed that prices were available, it finished upgrading vaults.
vat v48 upgraded from incarnation 0 to 1
Finally, we see "vault price updated" under Run Proposal Tests, which shows that the new vaultManager is receiving the updated prices provided via the new priceFeeds.

replace scaledPriceAuthority

This PR had been trying to add stTIA, stOSMO and stkATOM to the list of brands in A3P, to match those in mainNet, but that was a mistake. This drops those. It was previously adding priceFeeds, but not scaledPriceAuthorities.

Also generalize one test

closes: #9596 refs: #9748 ## Description When upgrading VaultFactory, restore the VaultDirector's parameters to the values that exist on chain. ### Security Considerations No particular security implications. ### Scaling Considerations Run only on upgrade. No impact. ### Documentation Considerations No user visible impact. ### Testing Considerations being tested manually thoroughly. The change to `typeParamManager.js` has a unit test. ### Upgrade Considerations Make it possible to preserve values that have been updated on chain. It would have been bad to lose the updated value of `ReferencedUI`.

dckc

I plan to take a fresh look once this is in the form of a core-eval rather than chain software upgrade (for example, once upgrade.go is gone)

LMK with the re-review button plz.

Chris-Hibbert · 2024-09-24T23:58:51Z

The changes here have been incorporated into #10074.

closes: #9584 closes: #9928 refs: #9827 refs: #9748 refs: #9382 closes: #10031 ## Description We added upgrading the scaledPriceAuthority to the steps in upgrading vaults, auctions, and priceFeeds, and didn't notice that it broke things. The problem turned out to be that the "priceAuthority" object registered with the priceFeedRegistry was an ephemeral object that was not upgraded. This fixes that by re-registering the new priceAuthority. Then, to simplify the process of cleaning up the uncollected cycles reported in #9483, we switched to replacing the scaledPriceAuthorities rather than upgrading them. We also realized that we would need different coreEvals in different environments, since the Oracle addresses and particular assets vary for each (test and mainNet) chain environment. #9748 addressed some of the issues in the original coreEval. #9999 showed what was needed for upgrading priceFeeds, which was completed by #9827. #10021 added some details on replacing scaledPriceAuthorities. ### Security Considerations N/A ### Scaling Considerations Addresses one of our biggest scaling issues. ### Documentation Considerations N/A ### Testing Considerations Thorough testing in a3p, and our testnets. #9886 discusses some testing to ensure Oracles will work with the upgrade. ### Upgrade Considerations See above

Chris-Hibbert added contract-upgrade oracle Related to on-chain oracles. Vaults VaultFactor (née Treasury) labels Jul 20, 2024

Chris-Hibbert self-assigned this Jul 20, 2024

Chris-Hibbert force-pushed the 9584-upgradeVaults branch from 3dd31b7 to d3217eb Compare July 31, 2024 20:05

Chris-Hibbert force-pushed the 9584-upgradeVaults branch from d3217eb to 0aa860e Compare July 31, 2024 20:08

Chris-Hibbert requested a review from turadg July 31, 2024 20:34

turadg reviewed Jul 31, 2024

View reviewed changes

Chris-Hibbert requested a review from dckc July 31, 2024 23:42

dckc reviewed Aug 1, 2024

View reviewed changes

turadg added the force:integration Force integration tests to run on PR label Aug 1, 2024

dckc mentioned this pull request Aug 1, 2024

chore: parameterize price feed replacement (check liquidation) #9827

Closed

2 tasks

Chris-Hibbert force-pushed the 9584-upgradeVaults branch from 87bd81d to 97fa744 Compare August 1, 2024 23:06

Chris-Hibbert mentioned this pull request Aug 2, 2024

feat: add the ability to override param values #9829

Merged

Chris-Hibbert force-pushed the 9584-upgradeVaults branch from fc11081 to 0442ba7 Compare August 2, 2024 17:15

Chris-Hibbert mentioned this pull request Aug 8, 2024

Reinstate scaledPriceAuthority upgrade; measure impact #9595

Closed

Chris-Hibbert requested a review from dckc August 8, 2024 00:49

dckc mentioned this pull request Aug 8, 2024

Figure out how to sequentialize vaults core evals #9841

Closed

Chris-Hibbert mentioned this pull request Aug 8, 2024

Measure allocations in priceFeeds #9867

Closed

Chris-Hibbert added 4 commits August 12, 2024 13:26

feat: repair vaultUpgrade proposal; register scaledPriceAuthority

567b9fb

replace scaledPriceAuthority

chore: minor cleanups from review

af752e1

test: A3P only has ATOM and stTIA. drop unknown tokens

e51ae60

This PR had been trying to add stTIA, stOSMO and stkATOM to the list of brands in A3P, to match those in mainNet, but that was a mistake. This drops those. It was previously adding priceFeeds, but not scaledPriceAuthorities.

chore: drop excess promise resolution, rotate priceAuthority names

36db8b2

Chris-Hibbert force-pushed the 9584-upgradeVaults branch from 0442ba7 to e69fe85 Compare August 12, 2024 22:02

chore: use scaledPriceAuthority's installation rather than label

659c016

Also generalize one test

Chris-Hibbert force-pushed the 9584-upgradeVaults branch from e69fe85 to 659c016 Compare August 12, 2024 22:37

dckc reviewed Aug 13, 2024

View reviewed changes

This was referenced Aug 14, 2024

Upgrade vaults and auctions with a coreEval #9887

Closed

Replace priceFeeds and scaledPriceAuthority #9928

Closed

Chris-Hibbert mentioned this pull request Sep 11, 2024

9584 upgrade price feeds #10074

Merged

Chris-Hibbert closed this Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repair vaultUpgrade proposal; also register replacement scaledPriceAuthority #9748

Repair vaultUpgrade proposal; also register replacement scaledPriceAuthority #9748

Chris-Hibbert commented Jul 20, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 31, 2024 •

edited

Loading

turadg left a comment

turadg Jul 31, 2024

Chris-Hibbert Jul 31, 2024

dckc Aug 1, 2024

Chris-Hibbert Aug 2, 2024

turadg Jul 31, 2024

Chris-Hibbert Jul 31, 2024

dckc Aug 1, 2024

Chris-Hibbert Aug 2, 2024

dckc left a comment

dckc Jul 31, 2024

dckc Jul 31, 2024

Chris-Hibbert Aug 12, 2024

dckc Aug 1, 2024

dckc Aug 1, 2024

Chris-Hibbert commented Aug 1, 2024

Chris-Hibbert commented Aug 1, 2024

dckc commented Aug 7, 2024

Chris-Hibbert commented Aug 8, 2024

dckc left a comment

Chris-Hibbert commented Sep 24, 2024

Repair vaultUpgrade proposal; also register replacement scaledPriceAuthority #9748

Repair vaultUpgrade proposal; also register replacement scaledPriceAuthority #9748

Conversation

Chris-Hibbert commented Jul 20, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jul 31, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert commented Aug 1, 2024

Chris-Hibbert commented Aug 1, 2024

dckc commented Aug 7, 2024

Chris-Hibbert commented Aug 8, 2024

dckc left a comment

Choose a reason for hiding this comment

Chris-Hibbert commented Sep 24, 2024

Chris-Hibbert commented Jul 20, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 31, 2024 •

edited

Loading